Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make sure we don't initialize mono before calling _start #12

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

mhmd-azeez
Copy link
Collaborator

@mhmd-azeez mhmd-azeez commented Oct 1, 2023

Currently, we are exporting an _initialize function that initializes the mono runtime. This is called by the host before calling the exported functions. However, the Rust SDK seems to call _initialize before _start too, which causes a problem because _start initializes the mono runtime too.

To fix this, I have changed the behavior of the PDK to NOT export _initialize anymore. Instead, we manually initialize the mono runtime before invoking any of the exported functions. This is inline with the behavior of some of the other compilers, namely: Rust and Haskell.

This has another added benefit: wasmtime refused to call plugins that had both _initialize and _start functions exported. This is no longer an issue since we never export _initialize anymore

@mhmd-azeez mhmd-azeez requested a review from zshipko October 1, 2023 11:52
@@ -14,11 +14,9 @@

void IsVowel(CurrentPlugin plugin, Span<ExtismVal> inputs, Span<ExtismVal> outputs, nint userData)
{
var bytes = plugin.ReadBytes(inputs[0].v.i32);
var c = (char)inputs[0].v.i32;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any need for a length check here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, the plugin won't be able to call the host function if they don't import it with the right signature

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok cool -- thank you!

Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zshipko or @bhelx would be better suited to give a final 👍 on this, but it LGTM

@mhmd-azeez mhmd-azeez merged commit 7462946 into master Oct 4, 2023
1 check passed
@mhmd-azeez mhmd-azeez deleted the fix/initialize branch October 4, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants